Fix subscriber losing App reset codes by kixelated · Pull Request #946 · moq-dev/moq

kixelated · 2026-02-12T15:23:25Z

Summary

Fixes #936. When a publisher aborts a track with Error::App(code), subscribers now correctly receive Error::App(code) instead of Error::Cancel.

Error::from_transport() checks stream_error() on transport errors to decode QUIC stream reset codes back into the original Error variant via the new from_code() method, instead of wrapping everything as Error::Transport
Subscribers no longer collapse Error::Transport into Error::Cancel, so decoded errors like Error::App propagate correctly to consumers
Simplified Error enum by removing inner data from Transport, Decode, Version, RequiredExtension, and BoundsExceeded variants, making the error type fully round-trippable through to_code()/from_code()

Test plan

just check passes (compilation, tests, linting, formatting)
End-to-end: publisher calls track.abort(Error::App(42)) → subscriber receives Error::App(42) (not Error::Cancel)

🤖 Generated with Claude Code

When a publisher aborts a track with Error::App(code), the subscriber now correctly receives Error::App(code) instead of Error::Cancel. The fix has two parts: 1. Error::from_transport() checks stream_error() on transport errors to decode QUIC stream reset codes back into the original Error variant via the new from_code() method, instead of wrapping everything as Error::Transport. 2. Subscribers no longer collapse Error::Transport into Error::Cancel, so decoded errors like Error::App propagate to consumers. Also simplifies Error by removing inner data from Transport, Decode, Version, RequiredExtension, and BoundsExceeded variants, making the error type fully round-trippable through to_code()/from_code(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

coderabbitai · 2026-02-12T15:29:42Z

Walkthrough

The PR refactors error representation and handling across the moq-lite crate: several Error variants (Transport, Decode, Version, RequiredExtension, BoundsExceeded) become unit-like, many new variants are added, and Error gains to_code, from_code, and from_transport conversions plus From impls. Call sites replace constructing Arc-wrapped transport errors with Error::from_transport. Reader adds decode_maybe and new has_more/read_more helpers. SessionInner switches from Arc to Box and its closed future no longer returns an error payload. Multiple modules adjust pattern matches and error mapping to the new error shape.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 48.72% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix subscriber losing App reset codes' directly addresses the main objective of this PR, which is fixing subscribers to correctly receive Error::App(code) instead of Error::Cancel when publishers abort with app errors.
Description check	✅ Passed	The description is well-related to the changeset. It explains the fix for issue `#936`, describes the three key changes (Error::from_transport, simplified Error enum, and subscriber error propagation), and outlines the test plan.
Linked Issues check	✅ Passed	The PR fully addresses the objectives from issue `#936`: Error::from_transport() decodes QUIC stream reset codes via new from_code() method, subscribers no longer collapse Error::Transport into Error::Cancel allowing Error::App to propagate, and the Error enum is simplified with unit-like variants making it round-trippable through to_code()/from_code().
Out of Scope Changes check	✅ Passed	All changes are scoped to the linked issue `#936`. The PR modifies error handling throughout the codebase to support decoding app error codes, which is directly required by the issue. No unrelated refactoring or feature additions were detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/subscriber-app-reset-codes

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

rs/moq-lite/src/error.rs (2)

83-137: to_code/from_code roundtrip is broken for UnknownAlpn

to_code maps UnknownAlpn(_) → 21, but from_code has no arm for 21, so it falls through to ProtocolViolation. This is likely fine since UnknownAlpn is a local ALPN-negotiation error (carrying a String that can't survive a u32 roundtrip), but it contradicts the PR objective of making errors "fully round-trippable."

Consider adding 21 => Self::UnknownAlpn(String::new()) or, alternatively, documenting that UnknownAlpn is intentionally local-only.

109-137: Consider adding unit tests for from_code/to_code roundtripping.

Given that this is the core mechanism for the bug fix (decoding stream reset codes back into the correct Error variant), inline tests would be valuable — especially to assert the App(code) roundtrip, boundary at code 64, and the fallback to ProtocolViolation for unknown codes.

As per coding guidelines, Rust tests should be integrated within source files.

rs/moq-lite/src/ietf/control.rs (1)

51-52: Minor semantic inconsistency between send failure and closed signaling.

send failure (line 51) maps to Error::Transport, while tx.closed() (line 90) returns Error::Cancel. Both indicate the receiver was dropped — i.e., the same underlying condition. Consider using Error::Cancel here for consistency, since the channel being closed is more of a cancellation than a transport failure.

Not a blocker — the practical impact is minimal since callers generally treat both as terminal.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

rs/moq-lite/src/error.rs (1)
77-102: ⚠️ Potential issue | 🟡 Minor

to_code can overflow for large App values.

App(u32::MAX).to_code() computes u32::MAX + 64, which panics in debug builds and wraps in release builds. Since App(u32) is public, callers can construct any value. Consider either clamping/saturating or validating at construction time.
Proposed fix: saturating arithmetic
-			Self::App(app) => *app + 64,
+			Self::App(app) => app.saturating_add(64),
Alternatively, you could cap App values in from_code or add a constructor that validates app < u32::MAX - 63.
rs/moq-lite/src/coding/reader.rs (1)
92-110: ⚠️ Potential issue | 🟠 Major

read_exact may loop infinitely if the stream closes before the buffer is filled.

The RecvStream::read_buf method returns Result<Option<usize>>, where Ok(None) signals EOF. On line 106, the result is only checked for errors via .map_err(Error::from_transport)?, but Ok(None) is silently discarded and the loop continues. This causes an infinite loop if the stream closes prematurely. The read_more() method (line 151) correctly handles this with a match statement that checks for Ok(None).
Proposed fix
 		while buf.has_remaining_mut() {
-			self.stream.read_buf(&mut buf).await.map_err(Error::from_transport)?;
+			match self.stream.read_buf(&mut buf).await {
+				Ok(Some(_)) => {}
+				Ok(None) => return Err(Error::Decode),
+				Err(e) => return Err(Error::from_transport(e)),
+			}
 		}

- Change Error::App(u32) to Error::App(u16) to prevent overflow when encoding to wire format (code + 64). Out-of-range codes in from_code() now return ProtocolViolation instead of silently truncating. - Fix pre-existing infinite loop in read_exact when the stream closes before the buffer is filled. read_buf returning Ok(None) was silently ignored; now returns Error::Decode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

roberte777

I think this looks good to me, basically what I came up with as well.

Integrate UnknownAlpn variant and Version enum refactor from main, keeping our simplified Error::Version (no inner data). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

coderabbitai bot reviewed Feb 12, 2026

View reviewed changes

roberte777 approved these changes Feb 12, 2026

View reviewed changes

Merge main and resolve conflicts

6b19b9f

Integrate UnknownAlpn variant and Version enum refactor from main, keeping our simplified Error::Version (no inner data). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

kixelated enabled auto-merge (squash) February 12, 2026 16:16

kixelated merged commit 3b74c03 into main Feb 12, 2026
1 check passed

kixelated deleted the fix/subscriber-app-reset-codes branch February 12, 2026 16:30

This was referenced Feb 12, 2026

chore: release #937

Merged

chore: release #950

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix subscriber losing App reset codes#946

Fix subscriber losing App reset codes#946
kixelated merged 3 commits intomainfrom
fix/subscriber-app-reset-codes

kixelated commented Feb 12, 2026

Uh oh!

coderabbitai bot commented Feb 12, 2026 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

roberte777 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kixelated commented Feb 12, 2026

Summary

Test plan

Uh oh!

coderabbitai bot commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

roberte777 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Feb 12, 2026 •

edited

Loading